-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(list): properly align contents in subheader #6221
fix(list): properly align contents in subheader #6221
Conversation
* The line-height of the subheaders is currently based on the typography level. This is problematic because the subheader is set to a specific height and can't grow/shrink accordingly. Fixes angular#6214
src/lib/list/_list-theme.scss
Outdated
@@ -46,7 +46,8 @@ | |||
} | |||
|
|||
.mat-subheader { | |||
@include mat-typography-level-to-styles($config, body-2); | |||
font: mat-font-weight($config, body-2) mat-font-size($config, body-2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The font
declaration is a little fiddly, especially when omitting values. I'd rather have this split into font-weight
, font-size
and font-family
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The font
property is used in this file for the dense mode as well (see here).
I'm fine changing it for both if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that. Also the font-family
could be set once on the .mat-list
and the subheader can inherit from there instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What about the dense attribute I also pointed out? Should I create a separate issue for that? Right now the same thing would happen in dense mode as there's only 8px left for the text. The spec doesn't even mention a dense subheader in dense mode. |
@grizzm0 This fix should also calculate the proper line-height for subheaders in dense mode. |
@devversion Yeah, but the height of subheader in dense mode is changed from 48px to 40px. (even tho the spec doesn't mention it). The padding is still 2x16px = 32px. Meaning there's only 8px left for the text while the font-size in this PR is caption (12px). That's 4px overflow which the text will be "pushed down" in dense mode. Edit: Ok, I see what you mean with the line-height. It will still "stick out" 2px on the top and 2px on the bottom tho. As you're trying to fit a 12px/8px font into an 8px high space. Hence me saying we're not supposed to change the height from 48px to 40px in dense mode in the subheader. Only on the items themselfs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #6214